-
-
Notifications
You must be signed in to change notification settings - Fork 981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use real path for module root dir (#6524) #7491
Conversation
💖 Thanks for opening this pull request! 💖 |
I am not sure why this is needed. We run
|
So I'll go deeper and find the source. Here is my analysis on where the faulty values comes from :
Somehow my scenario above, using this current PR don't catch we're at the parent level. |
config/config/src/index.ts
Outdated
@@ -291,7 +291,7 @@ export async function getConfig ( | |||
...rcOptions.map((configKey) => [camelcase(configKey), npmConfig.get(configKey)]) as any, // eslint-disable-line | |||
...Object.entries(cliOptions).filter(([name, value]) => typeof value !== 'undefined').map(([name, value]) => [camelcase(name), value]), | |||
]) as unknown as ConfigWithDeprecatedSettings | |||
const cwd = betterPathResolve(cliOptions.dir ?? npmConfig.localPrefix) | |||
const cwd = await fs.promises.realpath(betterPathResolve(cliOptions.dir ?? npmConfig.localPrefix)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make this operation synchronous as we don't run anything concurrently at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know for a fact you are already asynchronous really early in this function.
Also you do not start additional tasks so it should not be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be changed too. But not in this PR. Sync fs operations in Node.js are faster unless you can run them in parallel to something else.
Congrats on merging your first pull request! 🎉🎉🎉 |
Resolves #6524
Intro
I was not able to verify the fix with the mentioned
pd
command. I applied the same changes to this repository as I did on my local installation ofpnpm
for the issue to be fixed.I might not have fixed the original issue so I'll describe here the issue I faced and how I fixed it.
How to reproduce constantly the issue
parent
folder is either not existent or not importantMy analysis
At some point in a
pnpm add
command, There is a mismatch when fetching the manifest of animporter
because theirrootDir
uses the badParent
folder name instead of theparent
real folder name.I threw some logs right before that call (
console.log([Object.keys(manifestsByPath), rootDir])
) and I got the following log :I saw that you fetch the real path earlier in the function to check either or not the folder is part of the workspaces so I added the same and it worked !
Of course my local change was different as I acted on the compiled output and here is what I did :
(Change is at line 190518 of
pnpm.cjs
)const isFromWorkspace = is_subdir_1.default.bind(null, calculatedRepositoryRoot); importers = await (0, p_filter_1.default)(importers, async ({ rootDir }) => isFromWorkspace(await fs_1.promises.realpath(rootDir))); + importers = await Promise.all(importers.map(async ({ rootDir, ...rest }) => ({ rootDir: await fs_1.promises.realpath(rootDir), ...rest }))); if (importers.length === 0) return true;
Notes
If you have better knowledge of where this
rootDir
is from, you may want to update to therealpath
earlier and save other unidentified bugs related to it.